[WIP] "Not enough comments" #607 patch #646
Conversation
+ additions to impl.Main
| System.out.flush(); | ||
| // create a string variable called platformDependentExpectedResult which replaces all of the newlines in the constant parameter s of type string with the system line seperator, fetched by passing "line.seperator" to the method getProperty on class System | ||
| String platformDependentExpectedResult = s.replaceAll("\\n", System.getProperty("line.separator")); |
There was a problem hiding this comment.
Notes for anyone in the future; this line of code is currently not Enterprise Quality™ due to the fact that it does not import com.seriouscompany.business.java.fizzbuzz.packagenamingpackage.impl.Constants and use the constant LINE_SEPERATOR. To make this Enterprise Quality™, is it possible for somebody to start a pull request to change this singular line of code and make it follow industry standard by adding as many imports as possible to greatly optimize the code?
|
Please make sure this doesn't break compatibility with OS/2 and Windows NT 4.0, which are both used by our company. |
|
Please schedule a meeting with all of us so that we can align on whether this direction is the right way to go. Also, please deliver us a high level overview of the trade-offs and benefits you considered before deciding to go in this direction with these code changes. edit: Make sure to include latency considerations, as well, because we need to make sure we do this properly before we make any commitments. |
|
According to the company style guide, all single-line comments must start with a capital letter for maximal readability. Additionally if the comment is a complete sentence it should end with a period. Please update your PR accordingly. Thank you. |
samrland
changed the title
| import com.seriouscompany.business.java.fizzbuzz.packagenamingpackage.interfaces.strategies.FizzBuzzSolutionStrategy; | ||
|
|
||
| /** | ||
| * Standard FizzBuzz | ||
| */ | ||
| @Service | ||
| public class StandardFizzBuzz implements FizzBuzz { | ||
|
|
||
| // create a private contstant called _fizzBuzzSolutionStrategyFactory of type FizzBuzzSolutionStrategyFactory imported from com.seriouscompany.business.java.fizzbuzz.packagenamingpackage.interfaces.factories.FizzBuzzSolutionStrategyFactory on line 15 |
|
I like the idea but we should think about accessibility too. We can't just "add" comments with no regard to the languages the reader may or may not speak, can we add a translation feature to this? I've already invited everyone involved in here to bi-daily meetings of an hour, there we can discuss our plan of action, track the progress, which languages to support and maybe even determine what a language even is in the first place. |
Could you please see if this is already proposed in #549 ? |
Add comments.
This will close #607.
Original conversation was at #608; moved due to internal technical difficulties.